Skip to content

release-25.2: server: return draining progress if the server controller is draining #155640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 22, 2025
Merged

release-25.2: server: return draining progress if the server controller is draining #155640

merged 1 commit into from
Oct 22, 2025

Conversation

@iskettaneh
Copy link
Contributor

@iskettaneh iskettaneh commented Oct 17, 2025 •
edited
Loading

Backport 1/1 commits from #155063.

/cc @cockroachdb/release

This commit fixes a bug where if the server controller is being drained, the drain command would return a response without populating the IsDraining. The CLI interprets this as if the draining is completed.

server: return draining progress if the server controller is draining

This commit fixes a bug where if the server controller is being drained, the drain command would return a response without populating the IsDraining field. The CLI interprets this as if the draining is completed.

Output example when running:

./cockroach node drain 3 --insecure --url {pgurl:3} --logtostderr=INFO

I251008 15:54:42.613200 15 2@rpc/peer.go:613  [rnode=?,raddr=10.142.1.228:26257,class=system,rpc] 1  connection is now healthy
node is draining... remaining: 3
I251008 15:54:42.622586 1 cli/rpc_node_shutdown.go:184  [-] 2  drain details: tenant servers: 3
node is draining... remaining: 3
I251008 15:54:42.824526 1 cli/rpc_node_shutdown.go:184  [-] 3  drain details: tenant servers: 3
node is draining... remaining: 1
I251008 15:54:43.026405 1 cli/rpc_node_shutdown.go:184  [-] 4  drain details: tenant servers: 1
node is draining... remaining: 1
I251008 15:54:43.228596 1 cli/rpc_node_shutdown.go:184  [-] 5  drain details: tenant servers: 1
node is draining... remaining: 243
I251008 15:54:44.580413 1 cli/rpc_node_shutdown.go:184  [-] 6  drain details: liveness record: 1, range lease iterations: 175, descriptor leases: 67
node is draining... remaining: 0 (complete)
drain ok

Release note (bug fix): fixed a bug in the drain command where draining a node using virtual clusters (such as clusters running Physical Cluster Replication) could return before the drain was complete, possibly resulting in shutting down the node while it still had active SQL clients and ranges leases.

Epic: None

Release justification: need to backport a bug fix to draining where we used to not fully drain nodes when using virtual clustters.

@iskettaneh
server: return draining progress if the server controller is draining
3d124d0 
This commit fixes a bug where if the server controller is being drained,
the drain command would return a response without populating the
IsDraining field. The CLI interprets this as if the draining is
completed.

Output example when running:

```
./cockroach node drain 3 --insecure --url {pgurl:3} --logtostderr=INFO

I251008 15:54:42.613200 15 2@rpc/peer.go:613  [rnode=?,raddr=10.142.1.228:26257,class=system,rpc] 1  connection is now healthy
node is draining... remaining: 3
I251008 15:54:42.622586 1 cli/rpc_node_shutdown.go:184  [-] 2  drain details: tenant servers: 3
node is draining... remaining: 3
I251008 15:54:42.824526 1 cli/rpc_node_shutdown.go:184  [-] 3  drain details: tenant servers: 3
node is draining... remaining: 1
I251008 15:54:43.026405 1 cli/rpc_node_shutdown.go:184  [-] 4  drain details: tenant servers: 1
node is draining... remaining: 1
I251008 15:54:43.228596 1 cli/rpc_node_shutdown.go:184  [-] 5  drain details: tenant servers: 1
node is draining... remaining: 243
I251008 15:54:44.580413 1 cli/rpc_node_shutdown.go:184  [-] 6  drain details: liveness record: 1, range lease iterations: 175, descriptor leases: 67
node is draining... remaining: 0 (complete)
drain ok
```

Release note (bug fix): fixed a bug in the drain command where draining
a node using virtual clusters (such as clusters running Physical
Cluster Replication) could return before the drain was complete,
possibly resulting in shutting down the node while it still had active
SQL clients and ranges leases.

Epic: None
@iskettaneh iskettaneh requested review from a team as code owners October 17, 2025 17:53
@blathers-crl
Copy link

blathers-crl bot commented Oct 17, 2025

Thanks for opening a backport.

Before merging, please confirm that it falls into one of the following categories (select one):

  • Non-production code changes. Includes test-only changes, build system changes, etc.
  • Fixes for serious issues. Defined in the policy as correctness, stability, or security issues, data corruption/loss, significant performance regressions, breaking working and widely used functionality, or an inability to detect and debug production issues.
  • Other approved changes. These changes must be gated behind a disabled-by-default feature flag unless there is a strong justification not to.

Add a brief release justification to the PR description explaining your selection.

Also, confirm that the change does not break backward compatibility and complies with all aspects of the backport policy.

All backports must be reviewed by the TL and EM for the owning area.

@blathers-crl blathers-crl bot added backport Label PR's that are backports to older release branches T-kv KV Team labels Oct 17, 2025
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@blathers-crl
Copy link

blathers-crl bot commented Oct 17, 2025 •
edited
Loading

✅ PR #155640 is compliant with backport policy

Confidence: high
Critical bug criteria met: [Stability or security issues]
Backward compatible: true
Explanation: The PR fixes a critical bug related to the improper behavior of the drain command in nodes under specific virtual cluster scenarios, potentially leading to conditions where a node might be prematurely shut down while actively handling SQL clients and range leases. This correction addresses various criteria under the critical bug policy, such as stability issues and potentially improved performance by ensuring nodes drain entirely before shutting down. The PR has a release justification statement that explicitly states the need for this backport due to a bug affecting draining functionality in virtual clusters. Moreover, the changes are forward-compatible as there are no modifications to version gates and the behavior adjustments are scoped tightly to draining functionalities.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@iskettaneh iskettaneh requested a review from arulajmani October 17, 2025 19:55
@iskettaneh iskettaneh merged commit 6dde391 into cockroachdb:release-25.2 Oct 22, 2025
16 checks passed
@celiala
Copy link
Collaborator

celiala commented Oct 27, 2025

blathers backport staging-v25.2.8

@blathers-crl blathers-crl bot mentioned this pull request Oct 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevendanna stevendanna stevendanna approved these changes

@arulajmani arulajmani arulajmani approved these changes

Assignees

No one assigned

Labels

backport Label PR's that are backports to older release branches T-kv KV Team target-release-25.2.9

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iskettaneh @cockroach-teamcity @celiala @stevendanna @arulajmani